Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migrate to uWebSockets.js #583

Closed
wants to merge 1 commit into from

Conversation

shirtleton
Copy link

@shirtleton shirtleton commented May 17, 2019

This PR is not necessarily meant to merged but rather to continue the conversation with code rather than speculating and ghosting on issues. I believe a lot of us are waiting on some type of answer and personally, switching to ws as the background WebSocket engine is not an acceptable performance loss.

*Failing test was failing before changes were made.

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

uws is no longer maintained

New behaviour

Migrate to uWebSockets.js - the latest iteration of uws for the Node ecosystem.

Other information (e.g. related issues)

#578
#575
#567
#560
socketio/socket.io#3342

The internet

@ghost
Copy link

ghost commented May 30, 2019

UWS and uWebsocket.js has different structure and methods/getters/setters names.
UWS is compatible with ws, uWebsocket.js is not compatible, but is better and maintained.

My idea was to create wrappers for all classes uWebsocket.js offers to make it compatible with ws, then swaping will be the easiest part.

I guess it is not all that needs to be done:

An HttpRequest is stack allocated and only accessible during the callback invocation.
https://unetworking.github.io/uWebSockets.js/generated/interfaces/httprequest.html

engine.io keeps a copy of request in some cases, this also has to be taken into account

Also

uWebsocket.js offers general http requests by uSocket library that is faster than http module in node.js
I think it will be the best way to use it also for polling transport

@mmdevries
Copy link

@shirtleton we also had this problem. We didn't want to step over to the javascript ws package and we created a new uws package and added support for node 10 and node 12.
This new package is based on the original uws and we merged some fixes from cws.
Maybe this will help you as well.
https://github.com/mmdevries/uws

darrachequesne added a commit that referenced this pull request Jan 14, 2020
uws is no longer maintained, and does not support Node.js >= 10.

We could also try to support uWebSockets.js, but it does not have the same API as ws.

Source: https://github.com/mmdevries/uws
Related: #583
@frimuchkov
Copy link

Any news? Still waiting for merge :-)

@darrachequesne
Copy link
Member

@mmdevries thanks a lot for the hard work on this ❤️

I tried to use mmdevries/uws, but the test send and receive data with key and cert (ws) fails on Node.js 12. Do you have any idea why?

https://travis-ci.org/socketio/engine.io/jobs/637141561?utm_medium=notification&utm_source=github_status

The commit itself: 048c674

Node.js 8 & 10 are good though.

@darrachequesne
Copy link
Member

@Kamil93 were you able to create this wrapper? Could you please share it if that's the case?

darrachequesne added a commit that referenced this pull request Feb 6, 2020
uws is no longer maintained, and does not support Node.js >= 10.

We could also try to support uWebSockets.js, but it does not have the same API as ws.

Source: https://github.com/mmdevries/uws
Related: #583
darrachequesne added a commit that referenced this pull request Feb 6, 2020
uws is no longer maintained, and did not support Node.js >= 10.

We now use a fork in order to support the newer versions of Node.js.
It is maintained there: https://github.com/mmdevries/uws

And can be installed with: `npm i github:mmdevries/uws#2.4.1`

We could also try to support uWebSockets.js, but it does not have the
same API as ws.

The "engines" attribute has also been added in the package.json file,
since we broke the support for Node.js 6 in the latest minor release.

Source: https://github.com/mmdevries/uws
Related: #583
@mmdevries
Copy link

@darrachequesne there seemed to be some timing issue on node 12 which is solved now (version 2.4.2). Can you verify this?

@mmdevries
Copy link

@darrachequesne I've updated the node headers on version 12 and re-added support for node13. This should hopefully solve the problem. Version 2.9.11

@mmdevries
Copy link

There is a new npm module which you can try: eiows

@ghost
Copy link

ghost commented Jun 16, 2020

@mmdevries is it battle tested in some projects?

@mmdevries
Copy link

@Kamil93 we use it on many production systems already without problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants